Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JBERET-561 Change visibility of scopes implementations #232

Closed

Conversation

chengfang
Copy link
Contributor

@chengfang chengfang commented Jun 1, 2022

@luca-bassoricci
Copy link

@luca-bassoricci
Copy link

@liweinan
Copy link
Contributor

liweinan commented Dec 6, 2023

@chengfang Can I merge this PR now? In addition, can I port this into the main branch?

@chengfang
Copy link
Contributor Author

@liweinan changes in this PR by and large should still work. But watch out for recent changes in Quarkus api. As I noted in the related quarkus PR: "Also note that the latest quarkus InjectableContext interface has changed: there is now a destroy(ContextState) method replacing destroy(Contextual<?>)."

I was just searching for quarkus class InjectableContext, but couldn't find it anymore; it probably relocated or removed or just I didn't look carefully enough.

So need to make sure this PR will still work with latest, or recent versions of Quarkus that the user intends to support.

@luca-bassoricci
Copy link

@liweinan let me update my PR with latest Quarkus and JBeret code.
I'll ping you when I am done.

@luca-bassoricci
Copy link

@liweinan
quarkiverse/quarkus-jberet#123 (comment)
I also aligned code to latest jberet codebase but this PR is still bounded to pre-jakarta.
I'm waiting for @radcortez's response because I would like to avoid you to merge an incomplete PR.

@liweinan
Copy link
Contributor

liweinan commented Dec 7, 2023

@luca-bassoricci Thanks for the update! In addition do you want to add more commits into this PR?

@luca-bassoricci
Copy link

I haven't additional modification.
From my POV what has been done since now should be enough.

@liweinan
Copy link
Contributor

liweinan commented Dec 7, 2023

@luca-bassoricci Okay! If anything else need me to change please let me know.

@liweinan
Copy link
Contributor

@liweinan
Copy link
Contributor

@luca-bassoricci I'll be on PTO next week(and it will be two weeks holidays), if we can finish this by this weekend that would be great :D

@liweinan
Copy link
Contributor

btw I start to do the TCK testing for this PR. @chengfang

@luca-bassoricci
Copy link

@luca-bassoricci I'll be on PTO next week(and it will be two weeks holidays), if we can finish this by this weekend that would be great :D

On I have already prepared a pipeline (quarkiverse/quarkus-jberet@a38ed7b) to build the current main on my side and use the generated artifacts as dependency; I suppose I can use this PR branch but code need to be aligned to JBeret 2.x codebase before.

@liweinan
Copy link
Contributor

@luca-bassoricci Okay! When you are ready please let me know :)

Meanwhile I'll do the tck testing.

@luca-bassoricci
Copy link

On my side I'm ready: you have to align this PR to jberet 2.x because on quakus-jberet extension we dropped support for Quarkus 2.x in favor of Quarkus 3 (which use jakarta namespace).
If you don't want to merge I can use chengfang:scope-impl-public-JBERET-561 on my side to make a build and see if all is working fine; I just write quarkiverse/quarkus-jberet#123 (comment) to @radcortez for an advice because I have no experience about PR using external project as depencency.

let me know! 👍

@liweinan
Copy link
Contributor

@luca-bassoricci okay I'll work on my side once I finish testing I'll merge this and do the patching for other branches.

@liweinan liweinan force-pushed the scope-impl-public-JBERET-561 branch from ec8ffc7 to c3da5b2 Compare December 14, 2023 16:15
@liweinan
Copy link
Contributor

I have added CI test to the branch and I need to do the TCK testing for this PR. Today is too late for me, I'll try to finish working on this by this week(and also on branch 2.x together with the main branch). If I can't complete this by this weekend I'll finish working on this next week.

@liweinan
Copy link
Contributor

@luca-bassoricci If you don't need the patch on this branch anymore, I'll close it.

@luca-bassoricci
Copy link

@luca-bassoricci If you don't need the patch on this branch anymore, I'll close it.

Feel free to close it!

@liweinan liweinan closed this Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants